Add lintrunner pre-commit hook#18689
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/18689
Note: Links to docs will display an error until the docs builds have been completed. ❌ 1 New Failure, 1 Pending, 2 Unrelated FailuresAs of commit efc82a5 with merge base 19bbeac ( NEW FAILURE - The following job has failed:
FLAKY - The following jobs failed but were likely due to flakiness present on trunk:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
meta-cla
bot
added
the
CLA Signed
This PR needs a
|
abhinaykukkadapu
force-pushed
the
lintrunner-pre-commit-hook
branch
from
c2d602a to
298865a
Compare
|
I don't personally love commit hooks, as they can slow down iteration - esp. since lint w/ mypy can take a long time. I'm not opposed to landing it, but can we add a persistent local opt-out mechanism? |
| revision_flag="" | ||
| fi | ||
|
|
||
| lintrunner -a $revision_flag --skip MYPY |
There was a problem hiding this comment.
Remind me why skip MYPY?
There was a problem hiding this comment.
It is taking too much time and every time if the developer amends the commit, it might annoy them.
| cd -- "$( realpath "$( dirname -- "${BASH_SOURCE[0]}" )" )" &> /dev/null || /bin/true | ||
| ./run_python_script.sh ./install_executorch.py "$@" | ||
|
|
||
| # Install git hooks if inside a git repo |
There was a problem hiding this comment.
What if someone else (IIRC Arm) installs their own pre-commit hooks, would it collide with that?
Also Arm has more scripts here for push etc - https://github.com/pytorch/executorch/tree/19bbeac41ab4ba21aa95a44d464e72b8da571302/backends/arm/scripts
We should consolidate.
There was a problem hiding this comment.
@digantdesai good point, it seems their pre-commit hook is a subset of what we are doing and pre-push hook is very arm specific, i think we can consolidate the pre-commit but pre-push we should leave it as-is, because it has some sign-off checks, commit format etc which might surprise others.
What i will do is we can let the installation via install_executorch take precedence per hook, and honor the backend's own hooks via thin wrapper and call them if installed.
There was a problem hiding this comment.
Please also CC arm POCs if needed as fyi.
I disabled mypy for this reason and now it is pretty fast. I think the tradeoff is better than another loop of review -> lint fix -> review? Also, you can also bypass git commit hooks using |
abhinaykukkadapu
force-pushed
the
lintrunner-pre-commit-hook
branch
from
93af227 to
335f19b
Compare
Summary: Run lintrunner automatically on every commit via .githooks/pre-commit. Hooks are enabled by default through install_executorch.sh using core.hooksPath. Add a pre-push passthrough that delegates to .git/hooks/pre-push so backend-specific hooks (e.g., ARM) still work. Remove redundant ARM pre-commit hook. Test Plan: Tested all pre-commit branches (lintrunner missing, toml missing, first init, hash match, toml changed, init failure, first commit, lint pass, lint fail, sha256sum/shasum parity) — 18/18 passed.
abhinaykukkadapu
force-pushed
the
lintrunner-pre-commit-hook
branch
from
335f19b to
efc82a5
Compare
3 participants
Add lintrunner pre-commit hook
Summary:
Run lintrunner automatically on every commit via .githooks/pre-commit.
Hooks are enabled by default through install_executorch.sh.
Test Plan:
.lintrunner.tomlmissing: warns and skipslintrunner init, stores hash.lintrunner.tomlmodified: re-runs initlintrunner initfails: blocks commitsha256sum/shasumproduce same hash